-
Notifications
You must be signed in to change notification settings - Fork 66
Bxdf fixes cook torrance #930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
#ifndef __HLSL_VERSION | ||
namespace std | ||
{ | ||
template<typename T, uint16_t N> | ||
struct hash<nbl::hlsl::vector<T,N> > | ||
{ | ||
size_t operator()(const nbl::hlsl::vector<T,N>& v) const noexcept | ||
{ | ||
size_t seed = 0; | ||
NBL_UNROLL for (uint16_t i = 0; i < N; i++) | ||
nbl::core::hash_combine(seed, v[i]); | ||
return seed; | ||
} | ||
}; | ||
|
||
template<typename T> | ||
struct hash<nbl::hlsl::vector<T,1> > | ||
{ | ||
size_t operator()(const nbl::hlsl::vector<T,1>& v) const noexcept | ||
{ | ||
std::hash<T> hasher; | ||
return hasher(v.x); | ||
} | ||
}; | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure having this specialization globally is a good idea ?
What do you use it for ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also used in the bucket bxdf test, for hash map buckets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then define it in that example, nowhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this over from the test due to this comment
Devsh-Graphics-Programming/Nabla-Examples-and-Tests#165 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but don't make a specialization of std::hash
thats binding for everyone (different struct and name), esp that hlsl::vector
is a typedef of glm::vec[1,2,3,4]
and we're bound to introduce conflicts there with our future integrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it and used glm's std::hash
instead. Dunno why I missed that initially
struct reflect_wrapper | ||
{ | ||
vector3_type operator()() NBL_CONST_MEMBER_FUNC | ||
{ | ||
return r(VdotH); | ||
} | ||
bxdf::Reflect<scalar_type> r; | ||
scalar_type VdotH; | ||
}; | ||
reflect_wrapper r; | ||
r.r = bxdf::Reflect<scalar_type>::create(localV, localH); | ||
r.VdotH = cache.getVdotH(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write a comment that we're doing all these gymnastics to have reflect use an externally computed VdotH
such that it doesn't compute it itself again
reflect_wrapper r; | ||
r.r = bxdf::Reflect<scalar_type>::create(localV, localH); | ||
r.VdotH = cache.getVdotH(); | ||
ray_dir_info_type localL = localV_raydir.template reflect<reflect_wrapper>(r); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole point of the reflect_wrapper
was to avoid turning the reflection of V
around H
into a three step process:
- transforming fat
V
Ray-Dir into tangent space - reflecting
V
aroundH
to get fatL
- transforming tangent space fat
L
into original space of fatV
Note that we can do reflections and refractions in any space we like, so instead of bringing V to tangent and L back out of it, one can just bring H
to fat V
's space, and do the refraction using non-local V
and H
because cache.VdotH
remains invariant under rotations.
(this is why we get our reflection wrapper to override the computation of VdotH
, its not something compiler CSE can spot being identical)
This is not only 1 transformation instead of 2, but also a muuch cheaper transformation (always a 3x3 matrix mul with H
a vector, not a transformation of a direction vector and its derivatives or covariance matrices).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that creating a sample from worldspace L requires 3 dot products with TBN which is same as creating a sample from tangentspace L (TBN dot products already given) and transforming it into worldspace
HOWEVER:
- in isotropic BRDF usage, the TdotL and BdotL may be thrown away
- when derivatives and covariance matrices come, taking worldspace ray-dir info and doint 3 dot products will be cheaper
Also the NdotL
computation could be overwritten by 2.f*cache.getVdotH()*localH.z - localV.z
if you want (the quantitiy you check to see if the sample has invalid path).
const vector3_type localH = ndf.generateH(upperHemisphereV, u.xy); | ||
const vector3_type H = hlsl::mul(interaction.getFromTangentSpace(), localH); | ||
|
||
const scalar_type VdotH = hlsl::dot(V.getDirection(), H); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute this as dot(localV,localH)
before computing H
, it will shorten the instruction dependency cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also as soon as you know VdotH
you can check its sign's consistency with NdotV
, and reject the sample if it hits a microfacet from a different side than the surface
You can assert it, VNDF sampling should guarantee this
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV()); | ||
fresnel::OrientedEtaRcps<monochrome_type> rcpEta = _f.getOrientedEtaRcps(); | ||
|
||
ray_dir_info_type V = interaction.getV(); | ||
const vector3_type localV = interaction.getTangentSpaceV(); | ||
const vector3_type upperHemisphereV = ieee754::flipSignIfRHSNegative<vector3_type>(localV, hlsl::promote<vector3_type>(interaction.getNdotV())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assign interaction.getNdotV()
to a const variable, or better use localV.z
in its stead
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV()); | ||
fresnel::OrientedEtaRcps<monochrome_type> rcpEta = _f.getOrientedEtaRcps(); | ||
|
||
ray_dir_info_type V = interaction.getV(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need this till much later
Refract<scalar_type> r = Refract<scalar_type>::create(V.getDirection(), H); | ||
bxdf::ReflectRefract<scalar_type> rr; | ||
rr.refract = r; | ||
ray_dir_info_type L = V.reflectRefract(rr, transmitted, rcpEta.value[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a wrapper same as reflect to override operator()(const bool doRefract, const scalar_type rcpOrientedEta)
into actually calling operator()(const scalar_type NdotTorR, const scalar_type rcpOrientedEta)
which the first parameter coming from cache.LdotH
(N.B. the NdotI
internal computation will CSE with your VdotH
computation)
if ((ComputeMicrofacetNormal<scalar_type>::isTransmissionPath(VdotH, LdotH) != transmitted) || (LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0))) | ||
L.makeInvalid(); // should check if sample direction is invalid | ||
else | ||
cache = anisocache_type::create(VdotH, Ldir, H, T, B, _N, transmitted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use this instead
Nabla/include/nbl/builtin/hlsl/bxdf/common.hlsl
Lines 725 to 730 in 804014f
static this_t create( | |
const vector3_type tangentSpaceV, | |
const vector3_type tangentSpaceH, | |
const bool transmitted, | |
NBL_CONST_REF_ARG(fresnel::OrientedEtaRcps<monochrome_type>) rcpOrientedEta | |
) |
const vector3_type Ldir = L.getDirection(); | ||
const scalar_type LdotH = hlsl::dot(Ldir, H); | ||
if ((ComputeMicrofacetNormal<scalar_type>::isTransmissionPath(VdotH, LdotH) != transmitted) || (LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0))) | ||
L.makeInvalid(); // should check if sample direction is invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you actually want to make the isotropic part of the cache
first (before L
is computed) cause it computes VdotH
and LdotH
by itself without knowing the full L
continues https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files?diff=split&w=1#r2356451287
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The (this doesn't need to be checked, see next comment) LdotH
andNdotL
sign consistency could be checked right after computing transmitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're also checking the wrong thing, ComputeMicrofacetNormal<scalar_type>::isTransmissionPath(VdotH, LdotH) != transmitted
will always be true, why?
VNDF sampling guarantees that VdotH
will have a sign consistent with NdotV
, and transmitted
controls the sign of LdotH
relative to VdotH
by construction (you either reflect and end up with the same sign, or you refract and you get the opposite sign)
So what you should really be checking is isTransmissionPath(NdotV,NdotL) != transmitted
and the NdotL
should be computed in a friendly way similarly to this https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files/4faecc38efe93237e48b795402451366eeb1c5ba..804014f28e7469036673169266a013f42e018f60?diff=unified&w=1#r2401939155
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the above means that the (LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0))
check is redundant because when you know that
isTransmissionPath(NdotV,VdotH) === false // VNDF
and
isTransmissionPath(VdotH,LdotH) === transmitted // VNDF + construction from ReflectRefract
for sure, then
(LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0)) === (isTransmissionPath(NdotV,NdotL) != transmitted)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what you should assert() though after you pass the isTransmissionPath(NdotV,NdotL) != transmitted
check, is that
ComputeMicrofacetNormal::isValidMicrofacet(transmitted,cache.VdotL,localH.z,orientedEta)==true
else | ||
cache = anisocache_type::create(VdotH, Ldir, H, T, B, _N, transmitted); | ||
|
||
return sample_type::create(L, T, B, _N); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as for the BRDF generation, NdotL
can be overwriten by whatever you had to compute to check that L
is in the correct hemisphere as per transmitted
and the sign of VdotN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GH UI screwing with me again
template<typename C=bool_constant<!IsAnisotropic>, typename D=bool_constant<!IsBSDF> > | ||
enable_if_t<C::value && !IsAnisotropic && D::value && !IsBSDF, sample_type> generate(NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, const vector2_type u, NBL_REF_ARG(isocache_type) cache) | ||
{ | ||
anisocache_type aniso_cache; | ||
sample_type s = generate(anisotropic_interaction_type::create(interaction), u, aniso_cache); | ||
cache = aniso_cache.iso_cache; | ||
return s; | ||
} | ||
template<typename C=bool_constant<!IsAnisotropic>, typename D=bool_constant<IsBSDF> > | ||
enable_if_t<C::value && !IsAnisotropic && D::value && IsBSDF, sample_type> generate(NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, const vector3_type u, NBL_REF_ARG(isocache_type) cache) | ||
{ | ||
anisocache_type aniso_cache; | ||
sample_type s = generate(anisotropic_interaction_type::create(interaction), u, aniso_cache); | ||
cache = aniso_cache.iso_cache; | ||
return s; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could have just had one templated on V
used to type u
instead of D
if (IsBSDF || (_sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min)) | ||
{ | ||
scalar_type _pdf = __pdf<isotropic_interaction_type, isocache_type>(_sample, interaction, cache); | ||
return hlsl::mix(scalar_type(0.0), _pdf, _pdf < bit_cast<scalar_type>(numeric_limits<scalar_type>::infinity)); | ||
} | ||
else | ||
return scalar_type(0.0); | ||
} | ||
scalar_type pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(anisotropic_interaction_type) interaction, NBL_CONST_REF_ARG(anisocache_type) cache) | ||
{ | ||
if (IsBSDF || (_sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checks are not complete for a BSDF (same problem as eval
), not checking for TIR invalid microfacet or incosistent transmission with microfacets.
I'd really abstract the whole if (IsBSDF || (_sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min))
into a pseduo-private method (prefixed with __
) to use for eval
and pdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const bool notTIR = impl::check_TIR_helper<fresnel_type, IsBSDF>::template __call<MicrofacetCache>(_f, cache); | ||
if (notTIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quotient_and_pdf
is meant to be only called on samples we generate
d, so I'd turn these checks into asserts (also different thing needs to be asserted, see https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files#r2401699310 )
NBL_IF_CONSTEXPR(IsBSDF) | ||
quo = hlsl::promote<spectral_type>(G2_over_G1); | ||
else | ||
quo = _f(cache.getVdotH()) * G2_over_G1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert that the !(VdotH<0.f)
for certainty
|
||
dg1_query_type dq = ndf.template createDG1Query<Interaction, MicrofacetCache>(interaction, cache); | ||
|
||
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you only need to reorient fresnels for BSDFs, see https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files#r2401699310
quotient_pdf_type __quotient_and_pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache) | ||
{ | ||
scalar_type _pdf = __pdf<Interaction, MicrofacetCache>(_sample, interaction, cache); | ||
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only BSDFs need to re-orient
scalar_type DG = D.projectedLightMeasure; | ||
if (D.microfacetMeasure < bit_cast<scalar_type>(numeric_limits<scalar_type>::infinity)) | ||
{ | ||
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction); | ||
DG *= ndf.template correlated<sample_type, Interaction>(gq, _sample, interaction); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm actually since Eval is not Quotient, it should return 0 whenever PDF would have been INF, and therefore whenever Eval is inf, you can just return 0
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction); | ||
scalar_type G2_over_G1 = ndf.template G2_over_G1<sample_type, Interaction, MicrofacetCache>(gq, _sample, interaction, cache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gq
is not needed if the NDF is smooth, cause G2_over_G1
will approach 1
an NDF is smooth if PDF = INF
// set pdf=0 when quo=0 because we don't want to give high weight to sampling strategy that yields 0 contribution | ||
_pdf = hlsl::mix(_pdf, scalar_type(0.0), hlsl::all(quo < hlsl::promote<spectral_type>(numeric_limits<scalar_type>::min))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quotient will never be 0 after the _sample.valid()
is checked (valid microfacetness can be asserted), its only when a sample is invalid that you need to return {0,0}
Please take care of this #916 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more GH UI stuff
|
||
scalar_type G1_wo_numerator(scalar_type absNdotX, scalar_type NdotX2) | ||
{ | ||
return scalar_type(1.0) / (absNdotX + devsh_part(NdotX2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clamped
not abs
see #916 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't even have this method #916 (comment)
template<typename T> | ||
struct PhongExponent | ||
{ | ||
//conversion between alpha and Phong exponent, Walter et.al. | ||
static T ToAlpha2(T _n) | ||
{ | ||
return T(2.0) / (_n + T(2.0)); | ||
} | ||
//+INF for a2==0.0 | ||
static T FromAlpha2(T a2) | ||
{ | ||
return T(2.0) / a2 - T(2.0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make a BeckmannBase class that your Common inherits from, cause right now this is bxdf::ndf::PhongExponent
and people will think it can be used to translate phgon shininess into any NDF roughness
Description
Continues #899 , #916 and #919
Testing
TODO list: